Add attempt count metric to FaviconHandler

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 <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"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.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::Assign;
+using testing::Contains;
 using testing::ElementsAre;
 using testing::InSequence;
 using testing::Invoke;
 using testing::IsEmpty;
 using testing::Return;
 using testing::_;
 
 using IntVector = std::vector<int>;
 using URLVector = std::vector<GURL>;
 using BitmapVector = std::vector<SkBitmap>;
@@ skipping 37 matching lines @@
   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.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:
   struct Response {
     int http_status_code = 404;
     BitmapVector bitmaps;
     SizeVector original_bitmap_sizes;
   };
 
   FakeImageDownloader() : next_download_id_(1) {}
@@ skipping 210 matching lines @@
  protected:
   const std::vector<gfx::Size> kEmptySizes;
 
   // Some known icons for which download will succeed.
   const GURL kPageURL = GURL("http://www.google.com");
   const GURL kIconURL10x10 = GURL("http://www.google.com/favicon10x10");
   const GURL kIconURL12x12 = GURL("http://www.google.com/favicon12x12");
   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});
     delegate_.fake_downloader().Add(kIconURL16x16, IntVector{16});
     delegate_.fake_downloader().Add(kIconURL64x64, IntVector{64});
 
     // The score computed by SelectFaviconFrames() is dependent on the supported
     // scale factors of the platform. It is used for determining the goodness of
     // a downloaded bitmap in FaviconHandler::OnDidDownloadFavicon().
     // Force the values of the scale factors so that the tests produce the same
     // results on all platforms.
     scoped_set_supported_scale_factors_.reset(
         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_);
   }
 
   // Creates a new handler and feeds in the page URL and the candidates.
   // Returns the handler in case tests want to exercise further steps.
   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);
     // The first RunUntilIdle() causes the FaviconService lookups be faster than
     // OnUpdateFaviconURL(), which is the most likely scenario.
     base::RunLoop().RunUntilIdle();
     handler->OnUpdateFaviconURL(kPageURL, candidates);
     base::RunLoop().RunUntilIdle();
     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(
       const std::vector<GURL>& urls) {
     std::vector<favicon::FaviconURL> candidates;
     for (const GURL& url : urls) {
       candidates.emplace_back(url, FAVICON, kEmptySizes);
     }
     return RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
                                     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_;
 };
 
 TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   const GURL kIconURL("http://www.google.com/favicon");
 
   favicon_service_.fake()->Store(kPageURL, kIconURL,
                                  CreateRawBitmapResult(kIconURL));
 
   EXPECT_CALL(delegate_, OnFaviconUpdated(
                              kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
                              kIconURL, /*icon_url_changed=*/true, _));
 
   RunHandlerWithSimpleFaviconCandidates({kIconURL});
   EXPECT_THAT(delegate_.downloads(), IsEmpty());

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

This is probably a better test to check
download_histogram_->ExpectTotalCount("Favicons.DownloadAttempts", 0);

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

Added. (There are separate test cases, too, but this covers the 
cache-only case pretty well)

 }
 
 // Test that the FaviconHandler process finishes when:
 // - There is data in the database for neither the page URL nor the icon URL.
 // AND
 // - FaviconService::GetFaviconForPageURL() callback returns before
 //   FaviconHandler::OnUpdateFaviconURL() is called.
 // TODO(mastiz): Add test to verify UpdateFaviconMappingsAndFetch().
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
   EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
@@ skipping 195 matching lines @@
   // Defer the download completion such that RunUntilIdle() doesn't complete
   // the download.
   delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kIconURL1);
 
   delegate_.fake_downloader().Add(kIconURL1, IntVector{16});
   delegate_.fake_downloader().Add(kIconURL3, IntVector{64});
 
   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());
 
   // Favicon update should invalidate the ongoing download.
   EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL3, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL3, _, _));
 
   handler->OnUpdateFaviconURL(kPageURL,
                               {FaviconURL(kIconURL3, FAVICON, kEmptySizes)});
 
@@ skipping 19 matching lines @@
   // Defer the download completion such that RunUntilIdle() doesn't complete
   // the download.
   delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kSlowLoadingIconURL);
   delegate_.fake_downloader().Add(kSlowLoadingIconURL, IntVector{16});
 
   std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
 
   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());
 
   // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect,
   // despite the ongoing download.
   handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
   base::RunLoop().RunUntilIdle();
 
   // Complete the download.
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _));
   EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually());
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
 // Test that calling OnUpdateFaviconUrl() with the same icon URLs as before is a
 // no-op. This is important because OnUpdateFaviconUrl() is called when the page
 // finishes loading. This can occur several times for pages with iframes.
 TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinishedShouldBeNoop) {
   const std::vector<FaviconURL> favicon_urls = {
       FaviconURL(kIconURL10x10, FAVICON, kEmptySizes),
       FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
   };
 
   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.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
   handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty());
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
@@ skipping 248 matching lines @@
        FaviconURL(kIconURLWithoutSize1, FAVICON, kEmptySizes),
        FaviconURL(kIconURLWithoutSize2, FAVICON, kEmptySizes)});
 
   // Icon URLs are not registered and hence 404s will be produced, which
   // allows checking whether the icons were requested according to their size.
   // The favicons should have been requested in decreasing order of their sizes.
   // Favicons without any <link sizes=""> attribute should have been downloaded
   // last.
   EXPECT_THAT(delegate_.downloads(),
               ElementsAre(kIconURL1024_512, kIconURL16_512, kIconURL15_14,
                           kIconURLWithoutSize1, kIconURLWithoutSize2));

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

This is a good test to check

download_histogram_>ExpectBucketCount("Favicons.LargeIconDownloadAttempts",
                                      /*sample=*/1, /*expected_count=*/5);

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

Added a new test for multiple downloads. If you prefer, I can join them.

 }
 
 TEST_F(FaviconHandlerTest, TestSelectLargestFavicon) {
   const GURL kIconURL1("http://www.google.com/b");
   const GURL kIconURL2("http://www.google.com/c");
 
   delegate_.fake_downloader().Add(kIconURL1, IntVector{15});
   delegate_.fake_downloader().Add(kIconURL2, IntVector{14, 16});
 
   // Verify NotifyFaviconAvailable().
@@ skipping 49 matching lines @@
 
   RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_LARGEST,
       {FaviconURL(kIconURL10x10, FAVICON, SizeVector{gfx::Size(16, 16)}),
        FaviconURL(kIconURL12x12, FAVICON, SizeVector{gfx::Size(15, 15)}),
        FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
 }  // namespace
 }  // namespace favicon