Verify favicon bitmap content in FaviconHandler tests

components/favicon/core/favicon_handler_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/favicon/core/favicon_handler.h"
 
 #include <stddef.h>
 
 #include <map>
 #include <memory>
 #include <set>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/test_simple_task_runner.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 "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"
 #include "ui/gfx/image/image.h"
 
 namespace favicon {
 namespace {
 
 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;
 using testing::Invoke;
 using testing::IsEmpty;
 using testing::Return;
 using testing::_;
 
 using DownloadOutcome = FaviconHandler::DownloadOutcome;
 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 CreateBitmapWithEdgeSize(int 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;
+  }
+
+  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);
-
-  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);
-  }
+  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;
 }
 
 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;
   // Use a pixel size other than (0,0) as (0,0) has a special meaning.
   bitmap_result.pixel_size = gfx::Size(edge_size, edge_size);
   bitmap_result.icon_type = icon_type;
   bitmap_result.icon_url = icon_url;
   return {bitmap_result};
 }
 
@@ skipping 21 matching lines @@
     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;
   }
 
-  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;
     responses_[icon_url] = response;
   }
 
   // Disables automatic callback for |url|. This is useful for emulating a
   // download taking a long time. The callback for DownloadImage() will be
   // stored in |manual_callback_|.
   void SetRunCallbackManuallyForUrl(const GURL& url) {
@@ skipping 404 matching lines @@
 
   EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _));
 
   RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
 }
 
 // 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:
 // - There is valid data in the database for the page URL.
 // AND
 // - The icon URL used by the page has changed.
 // AND
 // - There is no data in database for the new icon URL.
 TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   const GURL kOldIconURL("http://www.google.com/old_favicon");
   const GURL kNewIconURL = kIconURL16x16;
 
   favicon_service_.fake()->Store(kPageURL, kOldIconURL,
                                  CreateRawBitmapResult(kOldIconURL));
 
   InSequence seq;
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kOldIconURL, _, _));
   EXPECT_CALL(favicon_service_, SetFavicons(_, kNewIconURL, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kNewIconURL, _, _));
 
   RunHandlerWithSimpleFaviconCandidates({kNewIconURL});
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kNewIconURL));
 }
 
 // If there is data for the page URL in history which is invalid, test that:
 // - The invalid data is not sent to the UI.
 // - The icon is redownloaded.
 TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
+  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
 // favicon URL to a favicon URL which is already cached in the database.
 TEST_F(FaviconHandlerTest, UpdateFavicon) {
   const GURL kSomePreviousPageURL("https://www.google.com/previous");
   const GURL kIconURL("http://www.google.com/favicon");
   const GURL kNewIconURL("http://www.google.com/new_favicon");
 
   favicon_service_.fake()->Store(kPageURL, kIconURL,
@@ skipping 459 matching lines @@
 TEST_F(FaviconHandlerTest, TestFaviconWasScaledAfterDownload) {
   const int kMaximalSize = FaviconHandler::GetMaximalIconSize(
       FaviconDriverObserver::NON_TOUCH_LARGEST);
 
   const GURL kIconURL1("http://www.google.com/b");
   const GURL kIconURL2("http://www.google.com/c");
 
   const int kOriginalSize1 = kMaximalSize + 1;
   const int kOriginalSize2 = kMaximalSize + 2;
 
-  delegate_.fake_downloader().AddWithOriginalSizes(
-      kIconURL1, IntVector{kMaximalSize}, IntVector{kOriginalSize1});
-  delegate_.fake_downloader().AddWithOriginalSizes(
-      kIconURL2, IntVector{kMaximalSize}, IntVector{kOriginalSize2});
+  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,
       {FaviconURL(kIconURL1, FAVICON,
                   SizeVector{gfx::Size(kOriginalSize1, kOriginalSize1)}),
        FaviconURL(kIconURL2, FAVICON,
                   SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})});
 
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL1));
 }
@@ skipping 161 matching lines @@
   RunHandlerWithSimpleFaviconCandidates({k404IconURL});
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
       ElementsAre(base::Bucket(static_cast<int>(DownloadOutcome::SKIPPED),
                                /*expected_count=*/1)));
 }
 
 }  // namespace
 }  // namespace favicon