Add metrics for quantity and type of Favicon candidates

components/favicon/content/content_favicon_driver_unittest.cc

Index: components/favicon/content/content_favicon_driver_unittest.cc
diff --git a/components/favicon/content/content_favicon_driver_unittest.cc b/components/favicon/content/content_favicon_driver_unittest.cc
index ad19e9523446f9d3d4b69f86267cca5bee0a873d..6571d85912e588483e83153e9f5d5cc224780f7d 100644
--- a/components/favicon/content/content_favicon_driver_unittest.cc
+++ b/components/favicon/content/content_favicon_driver_unittest.cc
@@ -5,14 +5,17 @@
 #include "components/favicon/content/content_favicon_driver.h"
 
 #include <memory>
+#include <vector>
 
 #include "base/macros.h"
+#include "base/test/histogram_tester.h"
 #include "components/favicon/core/favicon_client.h"
 #include "components/favicon/core/favicon_handler.h"
 #include "components/favicon/core/favicon_service.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/favicon_url.h"
 #include "content/public/test/test_renderer_host.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/gfx/favicon_size.h"
@@ -20,6 +23,8 @@
 namespace favicon {
 namespace {
 
+using testing::ElementsAre;
+
 class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
  public:
   ContentFaviconDriverTest() {}
@@ -126,6 +131,56 @@ TEST_F(ContentFaviconDriverTest, FaviconUpdateNoLastCommittedEntry) {
 
   // Test that ContentFaviconDriver ignored the favicon url update.
   EXPECT_TRUE(driver->favicon_urls().empty());
+  favicon_service()->Shutdown();
+}
+
+TEST_F(ContentFaviconDriverTest, RecordsHistorgramsForCandidates) {

[pkotwicz, 2017/02/27 18:54:57]

Nit: define a function local variable kSizes16x16and32x32 with the sizes 16x16
and 32x32. This will make the call on line 154 a bit more legible

[fhorschig, 2017/03/01 20:56:01]

Done.

+  base::HistogramTester tester;

[pkotwicz, 2017/02/27 18:54:57]

Move this initialization after line 158 to where it is used.

[fhorschig, 2017/03/01 20:56:01]

Done.

[pkotwicz, 2017/03/01 22:13:37]

Sorry for the confusion. I meant |favicon_urls|

[fhorschig, 2017/03/02 08:37:34]

Ah, okay, that makes more sense.

+  std::vector<content::FaviconURL> favicon_urls = {
+      content::FaviconURL(GURL("http://www.google.ca/favicon.ico"),
+                          content::FaviconURL::FAVICON,
+                          std::vector<gfx::Size>({{16, 16}, {32, 32}})),
+      content::FaviconURL(GURL("http://www.google.ca/precomposed_icon.png"),
+                          content::FaviconURL::TOUCH_PRECOMPOSED_ICON,
+                          std::vector<gfx::Size>()),

[pkotwicz, 2017/02/27 18:54:57]

Nit: Make use of kEmptyIconSizes that mastiz@ introduces in
https://codereview.chromium.org/2697803003/

[fhorschig, 2017/03/01 20:56:01]

Done.

+      content::FaviconURL(GURL("http://www.google.ca/large_favicon.png"),
+                          content::FaviconURL::TOUCH_ICON,
+                          std::vector<gfx::Size>())};
+  content::WebContentsObserver* driver_as_observer =
+      ContentFaviconDriver::FromWebContents(web_contents());
+
+  // Navigation to a page updating one icon.
+  NavigateAndCommit(GURL("http://www.youtube.com"));
+  driver_as_observer->DidUpdateFaviconURL(std::vector<content::FaviconURL>(

[pkotwicz, 2017/02/27 18:54:57]

I think that you can do
DidUpdateFaviconURL({}) instead of
DidUpdateFaviconURL(std::vector<content::FaviconURL>({}))

[fhorschig, 2017/03/01 20:56:01]

Done.

+      {content::FaviconURL(GURL("http://www.youtube.com/favicon.ico"),
+                           content::FaviconURL::FAVICON,
+                           std::vector<gfx::Size>({{16, 16}, {32, 32}}))}));
+
+  tester.ExpectTotalCount("Favicons.CandidatesCount", 1);
+  EXPECT_THAT(tester.GetAllSamples("Favicons.CandidatesCount"),
+              ElementsAre(base::Bucket(/*min=*/1, /*count=*/1)));
+  EXPECT_THAT(tester.GetAllSamples("Favicons.CandidatesWithDefinedSizesCount"),
+              ElementsAre(base::Bucket(/*min=*/1, /*count=*/1)));
+  EXPECT_THAT(tester.GetAllSamples("Favicons.CandidatesWithLargeIconsCount"),
+              ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
+
+  // Double navigation to a page with 3 different icons.
+  NavigateAndCommit(GURL("http://www.google.ca"));
+  driver_as_observer->DidUpdateFaviconURL(favicon_urls);
+  NavigateAndCommit(GURL("http://www.google.ca"));
+  driver_as_observer->DidUpdateFaviconURL(favicon_urls);
+
+  tester.ExpectTotalCount("Favicons.CandidatesCount", 3);

[pkotwicz, 2017/02/27 18:54:57]

Isn't this check redundant with assertion on line 174? Count of 3 = count of 1 +
count of 2

[fhorschig, 2017/03/01 20:56:01]

Done.

+  EXPECT_THAT(tester.GetAllSamples("Favicons.CandidatesCount"),
+              ElementsAre(base::Bucket(/*min=*/1, /*count=*/1),
+                          base::Bucket(/*min=*/3, /*count=*/2)));
+  EXPECT_THAT(tester.GetAllSamples("Favicons.CandidatesWithDefinedSizesCount"),
+              ElementsAre(base::Bucket(/*min=*/1, /*count=*/3)));
+  EXPECT_THAT(tester.GetAllSamples("Favicons.CandidatesWithLargeIconsCount"),
+              ElementsAre(base::Bucket(/*min=*/0, /*count=*/1),
+                          base::Bucket(/*min=*/2, /*count=*/2)));
+
+  favicon_service()->Shutdown();

[pkotwicz, 2017/02/27 18:54:57]

Can Shutdown() be called in the TearDown() function?

[fhorschig, 2017/03/01 20:56:01]

Gone. It's a mock (due to Mikels CL).

 }
 
 }  // namespace