Verify favicon bitmap content in FaviconHandler tests

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 584484a63939d9bf0be44a72f2578e118d3f89ae..43de4cbaa52ba75ed8698702c4c009d276b112bd 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -23,6 +23,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/layout.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
@@ -35,6 +36,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::Assign;
 using testing::ElementsAre;
 using testing::InSequence;
@@ -54,25 +56,35 @@ MATCHER_P2(ImageSizeIs, width, height, "") {
   return arg.Size() == gfx::Size(width, height);
 }
 
-// Fill the given bmp with some test data.
-SkBitmap CreateBitmapWithEdgeSize(int size) {
-  SkBitmap bmp;
-  bmp.allocN32Pixels(size, size);
+// |arg| is a SkBitmap.

[pkotwicz, 2017/05/08 17:11:16]

Isn't arg a gfx::Image?

[mastiz, 2017/05/09 08:29:45]

Fixed, thanks.

+MATCHER_P(ImageColorIs, expected_color, "") {
+  SkBitmap bitmap = arg.AsBitmap();
+  if (bitmap.empty()) {
+    *result_listener << "expected color but no bitmap data available";
+    return false;
+  }
 
-  unsigned char* src_data =
-      reinterpret_cast<unsigned char*>(bmp.getAddr32(0, 0));
-  for (int i = 0; i < size * size; 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);
+  SkColor actual_color = bitmap.getColor(1, 1);
+  if (actual_color != expected_color) {
+    *result_listener << "expected color "
+                     << base::StringPrintf("%08X", expected_color)
+                     << " but actual color is "
+                     << base::StringPrintf("%08X", actual_color);
+    return false;
   }
+  return true;
+}
+
+SkBitmap CreateBitmapWithEdgeSize(int size, SkColor color) {
+  SkBitmap bmp;
+  bmp.allocN32Pixels(size, size);
+  bmp.eraseColor(color);
   return bmp;
 }
 
 // Fill the given data buffer with valid png data.
-std::vector<unsigned char> FillBitmapWithEdgeSize(int size) {
-  SkBitmap bitmap = CreateBitmapWithEdgeSize(size);
+std::vector<unsigned char> FillBitmapWithEdgeSize(int size, SkColor color) {
+  SkBitmap bitmap = CreateBitmapWithEdgeSize(size, color);
   std::vector<unsigned char> output;
   gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &output);
   return output;
@@ -82,9 +94,10 @@ std::vector<FaviconRawBitmapResult> CreateRawBitmapResult(
     const GURL& icon_url,
     favicon_base::IconType icon_type = FAVICON,
     bool expired = false,
-    int edge_size = gfx::kFaviconSize) {
+    int edge_size = gfx::kFaviconSize,
+    SkColor color = SK_ColorRED) {
   scoped_refptr<base::RefCountedBytes> data(new base::RefCountedBytes());
-  data->data() = FillBitmapWithEdgeSize(edge_size);
+  data->data() = FillBitmapWithEdgeSize(edge_size, color);
   FaviconRawBitmapResult bitmap_result;
   bitmap_result.expired = expired;
   bitmap_result.bitmap_data = data;
@@ -126,23 +139,25 @@ class FakeImageDownloader {
     return download_id;
   }
 
-  void Add(const GURL& icon_url, const IntVector& sizes) {
-    AddWithOriginalSizes(icon_url, sizes, sizes);
-  }
-
-  void AddWithOriginalSizes(const GURL& icon_url,
-                            const IntVector& sizes,
-                            const IntVector& original_sizes) {
+  void Add(const GURL& icon_url,
+           const IntVector& sizes,
+           const IntVector& original_sizes,
+           SkColor color) {
     DCHECK_EQ(sizes.size(), original_sizes.size());
     Response response;
     response.http_status_code = 200;
     for (int size : sizes) {
       response.original_bitmap_sizes.push_back(gfx::Size(size, size));
-      response.bitmaps.push_back(CreateBitmapWithEdgeSize(size));
+      response.bitmaps.push_back(CreateBitmapWithEdgeSize(size, color));
     }
     responses_[icon_url] = response;
   }
 
+  // Simpler overload of the function above.
+  void Add(const GURL& icon_url, const IntVector& sizes) {
+    Add(icon_url, sizes, sizes, SK_ColorRED);
+  }
+
   void AddError(const GURL& icon_url, int http_status_code) {
     Response response;
     response.http_status_code = http_status_code;
@@ -567,22 +582,32 @@ TEST_F(FaviconHandlerTest, DownloadBookmarkedFaviconInIncognito) {
 // Test that the icon is redownloaded if the icon cached for the page URL
 // expired.
 TEST_F(FaviconHandlerTest, RedownloadExpiredPageUrlFavicon) {
+  const GURL kIconURL("http://www.google.com/favicon");
+  const SkColor kOldColor = SK_ColorBLUE;
+  const SkColor kNewColor = SK_ColorGREEN;
+
   favicon_service_.fake()->Store(
-      kPageURL, kIconURL16x16,
-      CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true));
+      kPageURL, kIconURL,
+      CreateRawBitmapResult(kIconURL, FAVICON, /*expired=*/true,
+                            gfx::kFaviconSize, kOldColor));
 
-  // TODO(crbug.com/700811): It would be nice if we could check whether the two
-  // OnFaviconUpdated() calls are called with different gfx::Images (as opposed
-  // to calling OnFaviconUpdated() with the expired gfx::Image both times).
-  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _)).Times(2);
-  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _));
+  delegate_.fake_downloader().Add(kIconURL, IntVector{gfx::kFaviconSize},
+                                  IntVector{gfx::kFaviconSize}, kNewColor);
 
-  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
-  // We know from the |kPageUrl| database request that |kIconURL16x16| has
-  // expired. A second request for |kIconURL16x16| should not have been made
-  // because it is redundant.
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL, _, _));

[pkotwicz, 2017/05/08 17:11:16]

Nit: We should probably check the color of the gfx::Image passed to
SetFavicons() too

[mastiz, 2017/05/09 08:29:45]

Done.

+
+  InSequence seq;
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, _, kIconURL, _, ImageColorIs(kOldColor)));
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, _, kIconURL, _, ImageColorIs(kNewColor)));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL});
+  // We know from the |kPageUrl| database request that |kIconURL| has expired. A
+  // second request for |kIconURL| should not have been made because it is
+  // redundant.
   EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
-  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL));
 }
 
 // Test that FaviconHandler requests the new data when:
@@ -611,22 +636,29 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
 // - The invalid data is not sent to the UI.
 // - The icon is redownloaded.
 TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
+  const GURL kIconURL("http://www.google.com/favicon");
+
+  delegate_.fake_downloader().Add(kIconURL, IntVector{gfx::kFaviconSize},
+                                  IntVector{gfx::kFaviconSize}, SK_ColorBLUE);
+
   // Set non empty but invalid data.
   std::vector<FaviconRawBitmapResult> bitmap_result =
-      CreateRawBitmapResult(kIconURL16x16);
+      CreateRawBitmapResult(kIconURL);
   // Empty bitmap data is invalid.
   bitmap_result[0].bitmap_data = new base::RefCountedBytes();
 
-  favicon_service_.fake()->Store(kPageURL, kIconURL16x16, bitmap_result);
+  favicon_service_.fake()->Store(kPageURL, kIconURL, bitmap_result);
 
-  // TODO(crbug.com/700811): It would be nice if we could check the image
-  // being published to rule out invalid data.
-  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _));
+  EXPECT_CALL(
+      delegate_,
+      OnFaviconUpdated(_, _, kIconURL, _,
+                       AllOf(ImageSizeIs(gfx::kFaviconSize, gfx::kFaviconSize),

[pkotwicz, 2017/05/08 17:11:15]

Why are we checking the size of the gfx::Image passed to OnFaviconUpdated()

[mastiz, 2017/05/09 08:29:45]

Removed. I thought it was worth verifying that the size if reported unmangled.

[pkotwicz, 2017/05/09 17:48:51]

Thank you for the explanation. I still think that we should not check for the
size.

I am hoping to minimize the number of assertions in the tests so that it is
clear what the tests are testing (as opposed to how the tests used to be circa a
few months ago)

+                             ImageColorIs(SK_ColorBLUE))));
 
-  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
+  RunHandlerWithSimpleFaviconCandidates({kIconURL});
 
   EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
-  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL));
 }
 
 // Test that no downloads are done if a user visits a page which changed its
@@ -1106,16 +1138,20 @@ TEST_F(FaviconHandlerTest, TestFaviconWasScaledAfterDownload) {
   const int kOriginalSize1 = kMaximalSize + 1;
   const int kOriginalSize2 = kMaximalSize + 2;
 
-  delegate_.fake_downloader().AddWithOriginalSizes(
-      kIconURL1, IntVector{kMaximalSize}, IntVector{kOriginalSize1});
-  delegate_.fake_downloader().AddWithOriginalSizes(
-      kIconURL2, IntVector{kMaximalSize}, IntVector{kOriginalSize2});
+  const SkColor kColor1 = SK_ColorBLUE;
+  const SkColor kColor2 = SK_ColorGREEN;
+
+  delegate_.fake_downloader().Add(kIconURL1, IntVector{kMaximalSize},
+                                  IntVector{kOriginalSize1}, kColor1);
+  delegate_.fake_downloader().Add(kIconURL2, IntVector{kMaximalSize},
+                                  IntVector{kOriginalSize2}, kColor2);
 
   // Verify the best bitmap was selected (although smaller than |kIconURL2|)
   // and that it was scaled down to |kMaximalSize|.
   EXPECT_CALL(delegate_,
               OnFaviconUpdated(_, _, kIconURL1, _,
-                               ImageSizeIs(kMaximalSize, kMaximalSize)));
+                               AllOf(ImageSizeIs(kMaximalSize, kMaximalSize),
+                                     ImageColorIs(kColor1))));

[pkotwicz, 2017/05/08 17:11:16]

Do we need to check the color of the gfx::Image? We are already checking the
icon URL

[mastiz, 2017/05/09 08:29:45]

Removed.

 
   RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_LARGEST,