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..34795ef8447d9d96ac82761899b298374b99972b 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,25 @@ bool IsValid(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   return bitmap_result.is_valid();
 }
 
+void RecordAttemptsForHandlerType(
+    FaviconDriverObserver::NotificationIconType handler_type,
+    int attempts) {
+  // The name of the histogram must not be a variable.
+  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;

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

Can we use the same histogram for Favicons.LargeIconDownloadAttempts and
Favicons.TouchIconDownloadAttempts

mastiz@ is trying to consolidate FaviconHandlers for type
FaviconDriverObserver::NON_TOUCH_LARGEST and
FaviconDriverObserver::TOUCH_LARGEST

in https://codereview.chromium.org/2795763004/

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

Done.

[fhorschig, 2017/04/10 13:59:41]

I was informed that we might have a large delay on that effort
and we could potentially lose the recorded data if that change
turns out to be pushed back. We discussed that it might make
more sense to consolidate the metrics later, which is still
wasy to do.
Therefore, I readded the differentiation for now. 
WDYT?

+  }
+  NOTREACHED();
+  return;

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

Nit: The return here is redundant

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

Gone.

+}
+
 // Returns true if |bitmap_results| is non-empty and:
 // - At least one of the bitmaps in |bitmap_results| is expired
 // OR
@@ -167,6 +187,7 @@ FaviconHandler::FaviconHandler(
       notification_icon_type_(favicon_base::INVALID_ICON),
       service_(service),
       delegate_(delegate),
+      request_attempts_(0),
       current_candidate_index_(0u) {
   DCHECK(delegate_);
 }
@@ -197,6 +218,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   got_favicon_from_history_ = false;
   download_request_.Cancel();
   candidates_.clear();
+  request_attempts_ = 0;
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
   current_candidate_index_ = 0u;
@@ -311,6 +333,7 @@ void FaviconHandler::OnUpdateFaviconURL(
 
   download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
+  request_attempts_ = 0;
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
@@ -399,6 +422,7 @@ void FaviconHandler::OnDidDownloadFavicon(
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
+    RecordAttemptsForHandlerType(handler_type_, request_attempts_);

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

Can you store |current_candidate_index_ + 1| instead?

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

Done. This captures 404s as attempts as well.

     // 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.
@@ -407,6 +431,7 @@ void FaviconHandler::OnDidDownloadFavicon(
     }
     // Clear download related state.
     current_candidate_index_ = candidates_.size();
+    request_attempts_ = 0;
     best_favicon_ = DownloadedFavicon();
   }
 }
@@ -533,6 +558,7 @@ void FaviconHandler::ScheduleDownload(const GURL& image_url,
   }
   download_request_.Reset(base::Bind(&FaviconHandler::OnDidDownloadFavicon,
                                      base::Unretained(this), icon_type));
+  ++request_attempts_;
   // A max bitmap size is specified to avoid receiving huge bitmaps in
   // OnDidDownloadFavicon(). See FaviconDriver::StartDownload()
   // for more details about the max bitmap size.