Add attempt count metric to FaviconHandler

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 5e651a646febcaca5d9b28ba8b03e129f4fb5da8..0d1877adc0bf1ebf7846a20ce8fd8aaf90573743 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -16,6 +16,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/histogram_tester.h"
 #include "components/favicon/core/favicon_driver.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -78,20 +79,21 @@ std::vector<unsigned char> FillBitmapWithEdgeSize(int size) {
 std::vector<FaviconRawBitmapResult> CreateRawBitmapResult(
     const GURL& icon_url,
     favicon_base::IconType icon_type = FAVICON,
-    bool expired = false) {
+    bool expired = false,
+    int edge_size = gfx::kFaviconSize) {
   scoped_refptr<base::RefCountedBytes> data(new base::RefCountedBytes());
-  data->data() = FillBitmapWithEdgeSize(gfx::kFaviconSize);
+  data->data() = FillBitmapWithEdgeSize(edge_size);
   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 = gfx::Size(edge_size, edge_size);
   bitmap_result.icon_type = icon_type;
   bitmap_result.icon_url = icon_url;
   return {bitmap_result};
 }
 
-// Fake that implements the calls to FaviconHalder::Delegate's DownloadImage(),
+// Fake that implements the calls to FaviconHandler::Delegate's DownloadImage(),
 // delegated to this class through MockDelegate.
 class FakeImageDownloader {
  public:
@@ -382,6 +384,7 @@ class FaviconHandlerTest : public testing::Test {
 };
 
 TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
+  base::HistogramTester histogram_tester;
   const GURL kIconURL("http://www.google.com/favicon");
 
   favicon_service_.fake()->Store(kPageURL, kIconURL,
@@ -393,6 +396,12 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
 
   RunHandlerWithSimpleFaviconCandidates({kIconURL});
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
+      IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.FaviconDownloadAttempts"),
+      IsEmpty());
 }
 
 // Test that UpdateFaviconsAndFetch() is called with the appropriate parameters
@@ -535,15 +544,12 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
 // - The icon is redownloaded.
 TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
   // Set non empty but invalid data.
-  FaviconRawBitmapResult bitmap_result;
-  bitmap_result.expired = false;
+  std::vector<FaviconRawBitmapResult> bitmap_result =
+      CreateRawBitmapResult(kIconURL16x16);
   // 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;
-  bitmap_result.icon_url = kIconURL16x16;
+  bitmap_result[0].bitmap_data = new base::RefCountedBytes();
 
-  favicon_service_.fake()->Store(kPageURL, kIconURL16x16, {bitmap_result});
+  favicon_service_.fake()->Store(kPageURL, kIconURL16x16, bitmap_result);
 
   // TODO(crbug.com/700811): It would be nice if we could check the image
   // being published to rule out invalid data.
@@ -1031,5 +1037,110 @@ TEST_F(FaviconHandlerTest, TestKeepDownloadedLargestFavicon) {
        FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
+TEST_F(FaviconHandlerTest, TestRecordMultipleDownloadAttempts) {
+  base::HistogramTester histogram_tester;
+
+  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)})});

[pkotwicz, 2017/04/10 17:26:10]

- Are multiple FaviconURLs helpful for this test?
- Are multiple sizes per FaviconURL helpful for this test?

If not can you simplify the input?

[fhorschig, 2017/04/11 12:07:21]

Yes, I want to log every single failed attempt. Commented.
Nope, removed.
Done.

+
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
+      ElementsAre(base::Bucket(/*sample=*/3, /*expected_count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.FaviconDownloadAttempts"),
+      IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.TouchIconDownloadAttempts"),
+      IsEmpty());
+}
+
+TEST_F(FaviconHandlerTest, TestRecordSingleFaviconDownloadAttempt) {
+  base::HistogramTester histogram_tester;
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP,
+      {FaviconURL(GURL("http://www.google.com/a"), FAVICON,
+                  {gfx::Size(1024, 1024), gfx::Size(512, 512)})});
+

[pkotwicz, 2017/04/10 17:26:10]

Are multiple sizes per FaviconURL helpful for this test?

[fhorschig, 2017/04/11 12:07:21]

Gone.

+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.FaviconDownloadAttempts"),
+      ElementsAre(base::Bucket(/*sample=*/1, /*expected_count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
+      IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.TouchIconDownloadAttempts"),
+      IsEmpty());
+}
+
+TEST_F(FaviconHandlerTest, TestRecordSingleLargeIconDownloadAttempts) {
+  base::HistogramTester histogram_tester;
+
+  RunHandlerWithCandidates(

[pkotwicz, 2017/04/10 17:26:10]

Nit: Can you use RunHandlerWithSimpleCandidates()

[fhorschig, 2017/04/11 12:07:21]

No, I want to set the NotificationIconType to NON_TOUCH_LARGEST.

+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(GURL("http://www.google.com/a"), FAVICON, kEmptySizes)});
+
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
+      ElementsAre(base::Bucket(/*sample=*/1, /*expected_count=*/1)));
+
+  // TOUCH_LARGEST fills the same bucket as NON_TOUCH_LARGEST.
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::TOUCH_LARGEST,
+      {FaviconURL(GURL("http://www.google.com/b"), TOUCH_ICON, kEmptySizes)});
+
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
+      ElementsAre(base::Bucket(/*sample=*/1, /*expected_count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.FaviconDownloadAttempts"),
+      IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.TouchIconDownloadAttempts"),
+      ElementsAre(base::Bucket(/*sample=*/1, /*expected_count=*/1)));
+}
+
+TEST_F(FaviconHandlerTest, TestRecordDownloadAttemptsFinishedByCache) {
+  // Names represent the bitmap sizes per icon.
+  const GURL kIconURL1024_512("http://www.google.com/a");
+  const GURL kIconURL15_14("http://www.google.com/b");
+  const GURL kIconURL16_512("http://www.google.com/c");

[pkotwicz, 2017/04/10 17:26:10]

Are multiple sizes per FaviconURL helpful for this test?

[fhorschig, 2017/04/11 12:07:21]

Gone.

+  base::HistogramTester histogram_tester;
+  favicon_service_.fake()->Store(
+      GURL("http://so.de"), kIconURL16_512,
+      {CreateRawBitmapResult(kIconURL16_512, FAVICON, /*expired=*/false,
+                             /*edge_size=*/16)[0],
+       CreateRawBitmapResult(kIconURL16_512, FAVICON, /*expired=*/false,
+                             /*edge_size=*/512)[0]});
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(kIconURL1024_512, FAVICON,
+                  {gfx::Size(1024, 1024), gfx::Size(512, 512)}),
+       FaviconURL(kIconURL15_14, FAVICON,
+                  {gfx::Size(15, 15), gfx::Size(14, 14)}),
+       FaviconURL(kIconURL16_512, FAVICON,
+                  {gfx::Size(16, 16), gfx::Size(512, 512)})});
+
+  // Should try only the first (receive 404) and get second icon from cache.
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL1024_512));
+
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
+      ElementsAre(base::Bucket(/*sample=*/1, /*expected_count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.FaviconDownloadAttempts"),
+      IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.TouchIconDownloadAttempts"),
+      IsEmpty());
+}
+
 }  // namespace
 }  // namespace favicon