Add DownloadStatus 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 bf88fd2c72483dac40a924224d63ffb8d6721f68..f44f9b45613ca001ae43f021b118a0809c95991f 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -42,6 +42,7 @@ 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>;
@@ -1062,7 +1063,7 @@ TEST_F(FaviconHandlerTest, TestRecordSingleFaviconDownloadAttempt) {
 
   RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_16_DIP,
-      {FaviconURL(GURL("http://www.google.com/a"), FAVICON,
+      {FaviconURL(GURL(kIconURL64x64), FAVICON,
                   {gfx::Size(1024, 1024), gfx::Size(512, 512)})});
 
   EXPECT_THAT(
@@ -1071,6 +1072,9 @@ TEST_F(FaviconHandlerTest, TestRecordSingleFaviconDownloadAttempt) {
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
       IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
+              ElementsAre(base::Bucket(DownloadOutcome::SUCCEEDED,
+                                       /*expected_count=*/1)));
 }
 
 TEST_F(FaviconHandlerTest, TestRecordSingleLargeIconDownloadAttempts) {
@@ -1078,7 +1082,7 @@ TEST_F(FaviconHandlerTest, TestRecordSingleLargeIconDownloadAttempts) {
 
   RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_LARGEST,
-      {FaviconURL(GURL("http://www.google.com/a"), FAVICON, kEmptySizes)});
+      {FaviconURL(GURL(kIconURL16x16), FAVICON, kEmptySizes)});
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
@@ -1087,7 +1091,7 @@ TEST_F(FaviconHandlerTest, TestRecordSingleLargeIconDownloadAttempts) {
   // TOUCH_LARGEST fills the same bucket as NON_TOUCH_LARGEST.
   RunHandlerWithCandidates(
       FaviconDriverObserver::TOUCH_LARGEST,
-      {FaviconURL(GURL("http://www.google.com/b"), TOUCH_ICON, kEmptySizes)});
+      {FaviconURL(GURL(kIconURL64x64), TOUCH_ICON, kEmptySizes)});
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.LargeIconDownloadAttempts"),
@@ -1095,6 +1099,9 @@ TEST_F(FaviconHandlerTest, TestRecordSingleLargeIconDownloadAttempts) {
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.FaviconDownloadAttempts"),
       IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
+              ElementsAre(base::Bucket(DownloadOutcome::SUCCEEDED,
+                                       /*expected_count=*/2)));
 }
 
 TEST_F(FaviconHandlerTest, TestRecordDownloadAttemptsFinishedByCache) {
@@ -1130,5 +1137,38 @@ TEST_F(FaviconHandlerTest, TestRecordDownloadAttemptsFinishedByCache) {
       IsEmpty());
 }
 
+TEST_F(FaviconHandlerTest, TestRecordSkippedDownloadAttemptAfterFailure) {

[pkotwicz, 2017/04/10 18:30:59]

Can you please split this test into two?
- One test for when UnableToDownloadFavicon(k404IconURL) returns true
- One test for when UnableToDownloadFavicon(k404IconURL) returns false

[fhorschig, 2017/04/11 12:24:53]

Done.

+  base::HistogramTester histogram_tester;
+  const GURL k404IconURL("http://www.google.com/404.png");
+
+  delegate_.fake_downloader().AddError(k404IconURL, 404);
+
+  // When calling an URL for the first time, it should log the failure.
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(k404IconURL));
+
+  RunHandlerWithCandidates(

[pkotwicz, 2017/04/10 18:30:59]

Can you use RunHandlerWithSimpleFaviconCandidates() ?

[fhorschig, 2017/04/11 12:24:53]

Done.

+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(GURL(k404IconURL), FAVICON, kEmptySizes)});
+
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
+      ElementsAre(base::Bucket(DownloadOutcome::FAILED, /*expected_count=*/1)));
+
+  // Now, the URL is known to be failing and should be skipped.
+
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(k404IconURL))
+      .WillByDefault(Return(true));
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(GURL(k404IconURL), FAVICON, kEmptySizes)});
+
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
+      ElementsAre(
+          base::Bucket(DownloadOutcome::FAILED, /*expected_count=*/1),
+          base::Bucket(DownloadOutcome::SKIPPED, /*expected_count=*/1)));
+}
+
 }  // namespace
 }  // namespace favicon