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 5a608b8ec5446bff4af2dfe69ff1338178c962a3..0afe6284d503d582a3e70ff20656ee428d4885eb 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -9,13 +9,16 @@
 #include <map>
 #include <memory>
 #include <set>
+#include <string>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/statistics_recorder.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"
@@ -34,6 +37,7 @@ using favicon_base::FaviconRawBitmapResult;
 using favicon_base::TOUCH_ICON;
 using favicon_base::TOUCH_PRECOMPOSED_ICON;
 using testing::Assign;
+using testing::Contains;
 using testing::ElementsAre;
 using testing::InSequence;
 using testing::Invoke;
@@ -91,7 +95,7 @@ std::vector<FaviconRawBitmapResult> CreateRawBitmapResult(
   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:
@@ -322,7 +326,7 @@ class FaviconHandlerTest : public testing::Test {
   const GURL kIconURL16x16 = GURL("http://www.google.com/favicon16x16");
   const GURL kIconURL64x64 = GURL("http://www.google.com/favicon64x64");
 
-  FaviconHandlerTest() {
+  FaviconHandlerTest() : download_histogram_(new base::HistogramTester()) {
     // Register various known icon URLs.
     delegate_.fake_downloader().Add(kIconURL10x10, IntVector{10});
     delegate_.fake_downloader().Add(kIconURL12x12, IntVector{12});
@@ -338,10 +342,22 @@ class FaviconHandlerTest : public testing::Test {
         new ui::test::ScopedSetSupportedScaleFactors({ui::SCALE_FACTOR_100P}));
   }
 
+  void TearDown() override {
+    // Check that if any downloads occurred, the attempt count was recorded.
+    if (!download_histogram_name_.empty() && delegate_.downloads().size() > 0) {

[pkotwicz, 2017/04/04 13:04:38]

We should check the histogram only for some tests not every test

[fhorschig, 2017/04/10 09:55:50]

Done.

+      download_histogram_->ExpectBucketCount(
+          download_histogram_name_,
+          /*sample=*/delegate_.downloads().size(),
+          /*expected_count=*/1);
+    }
+  }
+
   bool VerifyAndClearExpectations() {
     base::RunLoop().RunUntilIdle();
     favicon_service_.fake()->ClearDbRequests();
     delegate_.fake_downloader().ClearDownloads();
+    // Drop download histogram without verification.
+    download_histogram_ = base::MakeUnique<base::HistogramTester>();
     return testing::Mock::VerifyAndClearExpectations(&favicon_service_) &&
            testing::Mock::VerifyAndClearExpectations(&delegate_);
   }
@@ -351,6 +367,7 @@ class FaviconHandlerTest : public testing::Test {
   std::unique_ptr<FaviconHandler> RunHandlerWithCandidates(
       FaviconDriverObserver::NotificationIconType handler_type,
       const std::vector<favicon::FaviconURL>& candidates) {
+    download_histogram_name_ = GetHistogramForHandlerType(handler_type);
     auto handler = base::MakeUnique<FaviconHandler>(&favicon_service_,
                                                     &delegate_, handler_type);
     handler->FetchFavicon(kPageURL);
@@ -362,6 +379,20 @@ class FaviconHandlerTest : public testing::Test {
     return handler;
   }
 
+  std::string GetHistogramForHandlerType(
+      FaviconDriverObserver::NotificationIconType handler_type) {
+    switch (handler_type) {
+      case FaviconDriverObserver::NON_TOUCH_16_DIP:
+        return "Favicons.FaviconDownloadAttempts";
+      case FaviconDriverObserver::NON_TOUCH_LARGEST:
+        return "Favicons.LargeIconDownloadAttempts";
+      case FaviconDriverObserver::TOUCH_LARGEST:
+        return "Favicons.TouchIconDownloadAttempts";
+    }
+    NOTREACHED() << "Expect a histogram for every NotificationIconType!";
+    return std::string();
+  }
+
   // Same as above, but for the simplest case where all types are FAVICON and
   // no sizes are provided, using a FaviconHandler of type NON_TOUCH_16_DIP.
   std::unique_ptr<FaviconHandler> RunHandlerWithSimpleFaviconCandidates(
@@ -374,9 +405,11 @@ class FaviconHandlerTest : public testing::Test {
                                     candidates);
   }
 
+  std::string download_histogram_name_;
   base::MessageLoopForUI message_loop_;
   std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
       scoped_set_supported_scale_factors_;
+  std::unique_ptr<base::HistogramTester> download_histogram_;
   testing::NiceMock<MockFaviconServiceWithFake> favicon_service_;
   testing::NiceMock<MockDelegate> delegate_;
 };
@@ -608,6 +641,7 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   std::unique_ptr<FaviconHandler> handler =
       RunHandlerWithSimpleFaviconCandidates({kIconURL1, kIconURL2});
 
+  download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0);

[pkotwicz, 2017/04/04 13:04:38]

I don't think that this is a good test for this check

[fhorschig, 2017/04/10 09:55:50]

Gone.

   ASSERT_TRUE(VerifyAndClearExpectations());
   ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback());
 
@@ -647,6 +681,7 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessingShouldBeNoop) {
 
   ASSERT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kPageURL, kIconURL64x64, kSlowLoadingIconURL));
+  download_histogram_->ExpectTotalCount("Favicons.FaviconDownloadAttempts", 0);

[pkotwicz, 2017/04/04 13:04:38]

I don't think that this is a good test for this check

[fhorschig, 2017/04/10 09:55:50]

Gone.

   ASSERT_TRUE(VerifyAndClearExpectations());
   ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback());
 
@@ -675,6 +710,8 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinishedShouldBeNoop) {
   std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
 
+  download_histogram_->ExpectBucketCount("Favicons.FaviconDownloadAttempts",
+                                         /*sample=*/2, /*expected_count=*/1);

[pkotwicz, 2017/04/04 13:04:38]

I don't think that this is a good test for this check

[fhorschig, 2017/04/10 09:55:50]

Gone.

   ASSERT_TRUE(VerifyAndClearExpectations());
 
   // Calling OnUpdateFaviconURL() with identical data should be a no-op.