Add attempt count metric to FaviconHandler

components/favicon/core/favicon_handler.cc

Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 64c8961ae23ad760d06e826ee6e04bf06b7f1b2d..0fe9e7f892b2ed796c7cc3b3f8e263fee5cb4794 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon_base/favicon_util.h"
@@ -59,6 +60,23 @@ bool IsValid(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   return bitmap_result.is_valid();
 }
 
+void RecordAttemptsForHandlerType(
+    FaviconDriverObserver::NotificationIconType handler_type,
+    int attempts) {
+  switch (handler_type) {
+    case FaviconDriverObserver::NON_TOUCH_16_DIP:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.FaviconDownloadAttempts", attempts);
+      return;
+    case FaviconDriverObserver::NON_TOUCH_LARGEST:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", attempts);
+      return;
+    case FaviconDriverObserver::TOUCH_LARGEST:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.TouchIconDownloadAttempts", attempts);
+      return;
+  }
+  NOTREACHED();
+}
+
 // Returns true if |bitmap_results| is non-empty and:
 // - At least one of the bitmaps in |bitmap_results| is expired
 // OR
@@ -399,6 +417,7 @@ void FaviconHandler::OnDidDownloadFavicon(
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
+    RecordAttemptsForHandlerType(handler_type_, current_candidate_index_ + 1);
     // We have either found the ideal candidate or run out of candidates.
     if (best_favicon_.candidate.icon_type != favicon_base::INVALID_ICON) {
       // No more icons to request, set the favicon from the candidate.
@@ -503,6 +522,7 @@ void FaviconHandler::OnFaviconData(const std::vector<
     // the observers even if the favicon is expired or incomplete (incorrect
     // size) because temporarily showing the user an expired favicon or
     // streched favicon is preferable to showing the user the default favicon.
+    RecordAttemptsForHandlerType(handler_type_, current_candidate_index_);

[pkotwicz, 2017/04/10 14:58:11]

Why are we recording Favicons.FaviconDownloadAttempts from here?

[fhorschig, 2017/04/10 15:05:21]

Because TestRecordDownloadAttemptsFinishedByCache would fail.

We have multiple candidates, but the best candidate fails to load.
Then the cached next candidate is used to complete the request.
I considered it important to capture these attempts as well.

[pkotwicz, 2017/04/10 17:26:10]

Sorry for the confusion earlier. I think that we need a dedicated variable which
counts the number of downloads: |num_download_requests_| (because that is what
the histogram is recording)

We should probably only record the metric if |num_download_requests_| is non
zero

Sorry for flip flopping

[fhorschig, 2017/04/11 12:07:20]

I agree, It's more explicit and future-proof in we ever skip candidates.
The recorded number is still exactly the same (incl. 404 as attempts).
Done. Added a check in |OnFaviconData| so |RecordAttempts...| does the action
it implies. Added comment to |OnDidDownload...|.
Don't worry.

     NotifyFaviconUpdated(favicon_bitmap_results);
   }