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 1ed8333b061954264f02c7fe29288cdc05042090..172f321dd78af559cdd5b44718d3c77bfd27e33f 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -11,6 +11,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"
@@ -209,6 +210,7 @@ FaviconHandler::FaviconHandler(
       notification_icon_type_(favicon_base::INVALID_ICON),
       service_(service),
       delegate_(delegate),
+      request_attempts_(0),
       current_candidate_index_(0u) {
   DCHECK(delegate_);
 }
@@ -238,6 +240,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
   download_request_.Cancel();
+  request_attempts_ = 0;
   image_urls_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
@@ -372,6 +375,7 @@ void FaviconHandler::OnUpdateFaviconURL(
   }
 
   download_request_.Cancel();
+  request_attempts_ = 0;
   image_urls_ = pruned_candidates;
   current_candidate_index_ = 0u;
   best_favicon_candidate_ = FaviconCandidate();
@@ -473,6 +477,12 @@ void FaviconHandler::OnDidDownloadFavicon(
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
     // We have either found the ideal candidate or run out of candidates.
+    if (handler_type_ == FaviconDriverObserver::TOUCH_LARGEST) {
+      UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts",

[jkrcal, 2017/03/30 07:53:28]

Can we actually have three histogram per each
FaviconDriverObserver::NotificationIconType?
(can you also have the naming of the histograms consistent with the naming of
the enum?)

[fhorschig, 2017/04/03 09:56:49]

I added a third historgram but it's slightly different:
If we ever add new type to the enum without new metric, we would lose the total
number of attempts. If we have the overall count and counts for Large and Touch
icon downloads, we can calculate the number of attempts for the small favicons.

We could also add a fourth type for small icons, but I don't think it's
important
enough to record duplicate metrics. WDYT?

+                               request_attempts_);
+    } else {
+      UMA_HISTOGRAM_COUNTS_100("Favicons.DownloadAttempts", request_attempts_);
+    }
     if (best_favicon_candidate_.icon_type != favicon_base::INVALID_ICON) {
       // No more icons to request, set the favicon from the candidate.
       SetFavicon(best_favicon_candidate_.image_url,
@@ -481,6 +491,7 @@ void FaviconHandler::OnDidDownloadFavicon(
     }
     // Clear download related state.
     current_candidate_index_ = image_urls_.size();
+    request_attempts_ = 0;
     best_favicon_candidate_ = FaviconCandidate();
   }
 }
@@ -598,6 +609,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.