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 <vector>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #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"
 #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"
 
@@ skipping 42 matching lines @@
 std::vector<unsigned char> FillBitmapWithEdgeSize(int size) {
   SkBitmap bitmap = CreateBitmapWithEdgeSize(size);
   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) {
+    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:
   struct Response {
     int http_status_code = 404;
     BitmapVector bitmaps;
     SizeVector original_bitmap_sizes;
   };
 
   FakeImageDownloader() : next_download_id_(1) {}
@@ skipping 270 matching lines @@
   }
 
   base::MessageLoopForUI message_loop_;
   std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
       scoped_set_supported_scale_factors_;
   testing::NiceMock<MockFaviconServiceWithFake> favicon_service_;
   testing::NiceMock<MockDelegate> delegate_;
 };
 
 TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
+  base::HistogramTester histogram_tester;
   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());
+  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
 // when there is data in the database for neither the page URL nor the icon URL.
 TEST_F(FaviconHandlerTest, UpdateFaviconMappingsAndFetch) {
   EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
                                     kPageURL, URLVector{kIconURL16x16}, FAVICON,
                                     /*desired_size_in_dip=*/16, _, _));
 
   RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
@@ skipping 122 matching lines @@
 
   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) {
   // 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.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _));
 
   RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
 
   EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
 }
@@ skipping 467 matching lines @@
   EXPECT_CALL(delegate_,
               OnFaviconUpdated(_, _, kIconURL12x12, _, ImageSizeIs(12, 12)));
 
   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)});
 }
 
+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